Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RSDK-9288: declutter UI #4692

Merged
merged 41 commits into from
Jan 16, 2025
Merged

RSDK-9288: declutter UI #4692

merged 41 commits into from
Jan 16, 2025

Conversation

purplenicole730
Copy link
Member

@purplenicole730 purplenicole730 commented Jan 8, 2025 •
edited
Loading

Changes:

  • hide help command for all commands with a subcommand list
  • fix incorrect usage text and remove arguments from usage text
  • standardize usage text to better reflect what's required and optional
  • remove arguments and replace with flags, keep backwards capability
    • left machines part cp command alone since it's using the same logic as the regular CLI cp command, which most users should be more comfortable with
  • consolidate flags
  • generalized helper functions to be more intuitive to use

Most of the changes can be shown through the following examples.
Before: command is unclear if it's supposed to be replaced or not, and arguments is misleading

rdk % go run ./cli/viam --help                 
USAGE:
   viam [global options] command [command options] [arguments...]

After: remove arguments and highlight command is something to replace:

rdk % go run ./cli/viam --help
USAGE:
   viam [global options] <command> [command options]

Before: ignores login options and ignores that command is optional

rdk % go run ./cli/viam login --help
USAGE:
   viam login command [command options] [arguments...]

After: alludes to login options and command is now optional

rdk % go run ./cli/viam login --help
USAGE:
   viam login [options] [command] [command options]

Before: Incorrectly shows additional options and arguments when there are none

rdk % go run ./cli/viam login print-access-token --help
USAGE:
   viam login print-access-token [command options] [arguments...]

After: Show clear and correct usage

rdk % go run ./cli/viam login print-access-token --help
USAGE:
   viam login print-access-token

Before: incorrect command name and alludes to nonexistent other options

rdk % go run ./cli/viam organizations auth-service enable --help
USAGE:
   viam enable --org-id=<org-id> [other options]

After: correct way to use command

rdk % go run ./cli/viam organizations auth-service enable --help
USAGE:
   viam organizations auth-service enable --org-id=<org-id>

Before: shows wrong usage of org-id flag and ignores subcommands

rdk % go run ./cli/viam organizations logo --help
USAGE:
   viam organizations logo --org-id=<org-id> [other options]

COMMANDS:
   set      set the logo for an organization from a local file
   get      get the logo for an organization
   help, h  Shows a list of commands or help for one command

After: correct usage of logo without flags and with subcommands. hides help subcommand

rdk % go run ./cli/viam organizations logo --help               
USAGE:
   viam organizations logo <command> [command options]

COMMANDS:
   set  set the logo for an organization from a local file
   get  get the logo for an organization

Before: Alludes to command options that don't exist, inconsistent org argument

rdk % go run ./cli/viam locations list --help
USAGE:
   viam locations list [command options] [organization]

OPTIONS:
   --help, -h  show help (default: false)

After: add org flag (which we kept in usagetext as that's the default usage), remove arguments suggestion

rdk % go run ./cli/viam locations list --help
USAGE:
   viam locations list [--organization=<organization>]

OPTIONS:
   --organization value, --org value, --org-id value, --org-name value  (default: first organization alphabetically)

Test to make sure:

  • default continues working
  • flag works
  • argument works for backwards capability
  • flag takes priority over argument
rdk % go run ./cli/viam locations list                     
Locations for "<>":
<>:
        First Location (id: <>)
rdk % go run ./cli/viam locations list --organization "<>"
        First Location (id: <>)
rdk % go run ./cli/viam locations list "<>"               
        First Location (id: <>)
rdk % go run ./cli/viam locations list --organization "<>" "fake"
        First Location (id: <>)

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
purplenicole730
purplenicole730 commented Jan 16, 2025
View reviewed changes
cli/client.go
Comment on lines -1219 to -1263
// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
// Move this to being a flag (but make sure existing workflows still work!)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to keep this specific function as is, as it mirrors a well-known behavior with the cp command

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
stuqdog
stuqdog approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing and a flyby request, otherwise lgtm!

cli/app.go
HideHelpCommand: true,
Subcommands: []*cli.Command{
{
Name: "list",
Usage: "list locations for the current user",
ArgsUsage: "[organization]",
Action: createCommandWithT[emptyArgs](ListLocationsAction),
UsageText: "viam locations list [--organization=<organization>]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) can we maybe add a quick comment here just explaining why we aren't using the helper function?

cli/client.go
@@ -1216,8 +1223,6 @@ func machinesPartCopyFilesAction(
flagArgs machinesPartCopyFilesArgs,
logger logging.Logger,
) error {
// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(flyby request) I know you didn't make this change but the definition of machinesPartCopyFilesAction here takes a *viamClient as an argument. Would you mind, as a quick flyby, making it a method on a viamClient instead of taking the viamClient as an argument? This would put the syntax more in line with how analogous functions elsewhere in the CLI are structured.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@purplenicole730 purplenicole730 merged commit 61f77ed into viamrobotics:main Jan 16, 2025
16 checks passed
@purplenicole730 purplenicole730 deleted the RSDK-9288-declutter-UI branch January 16, 2025 20:21
@purplenicole730 purplenicole730 restored the RSDK-9288-declutter-UI branch January 16, 2025 20:21
@purplenicole730 purplenicole730 deleted the RSDK-9288-declutter-UI branch January 16, 2025 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuqdog stuqdog stuqdog approved these changes

@lia-viam lia-viam Awaiting requested review from lia-viam lia-viam is a code owner automatically assigned from viamrobotics/sdk-netcode

Assignees
No one assigned
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@purplenicole730 @stuqdog @viambot